Guard eval in Dashboard test harness; fix rollup build-mode warning suppression - #762
Merged
Merged
Conversation
…uard; fix rollup build-mode warning suppression clickHandlers.js: doEvaluateString backs the Dashboard's internal React test suite, which needs to call arbitrary NotePlan API methods from the webview side to set up test fixtures - hence the eval. The webview only ever loads this plugin's own bundled local JS (no remote content, no note content eval'd/rendered unescaped), so reaching this handler requires either being the trusted test code or an attacker who has already achieved script execution via some unrelated XSS bug - in which case this would let them escalate from webview JS to the privileged backend JSContext. The UI entry point (Dashboard.jsx's showDebugPanel) was already gated behind _logLevel === 'DEV' + FFlag_DebugPanel, but that only hid the button - the bridge handler itself had no runtime check. Added a matching guard directly in doEvaluateString so the protection isn't just a warning comment. scripts/rollup.js: the build-mode path (-b/-nc, used by `npc plugin:dev <id> -nc` and CI) manually reconstructed `inputOptions` from getConfig() picking only external/input/plugins/context/cache - silently dropping the onwarn handler that suppresses EVAL and MODULE_LEVEL_DIRECTIVE warnings. Watch mode spreads the full config so it was never affected. This is what caused the "Use of eval... strongly discouraged" warning to appear during `-nc` builds even though onwarn was coded to suppress it years ago. Added the missing `onwarn: options.onwarn` to the build-mode inputOptions so both paths behave consistently. Verified: full test suite passes (198 suites, 4553 tests); flow error count on clickHandlers.js unchanged (16, confirmed via stash comparison); jgclark.Dashboard builds clean with no eval warning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small, unrelated fixes discovered while investigating a build warning:
1.
jgclark.Dashboard/src/clickHandlers.js— document + guardevalusagedoEvaluateStringbacks the Dashboard's internal React test suite (src/react/components/testing/*.tests.js), which needs to call arbitrary NotePlan API methods (e.g.Editor.openNoteByFilename(...)) from the webview side to set up test fixtures. That's why it evals a string in the backend JSContext.Reachability: the Dashboard webview only ever loads this plugin's own bundled local JS — no remote content, no note content is eval'd or rendered unescaped into the webview. So this can only be reached today by the plugin's own trusted test code, or by an attacker who has already achieved script execution inside the webview via some unrelated XSS bug — in which case it would let them escalate from webview-only JS to the privileged backend JSContext.
The UI entry point (
Dashboard.jsx'sshowDebugPanel) was already gated behind_logLevel === 'DEV'+FFlag_DebugPanel, but that only hid the button — the bridge handler itself (pluginToHTMLBridge.js'scase 'evaluateString') had no runtime check, so anything able to construct a bridge message could reacheval()regardless of dev mode. Added a matching guard directly insidedoEvaluateString, plus a detailed comment explaining why eval is used and what the actual risk surface is, so the protection isn't just a warning comment.2.
scripts/rollup.js— fix build-mode warning suppressionThe build-mode path (
-b/-nc, used bynpc plugin:dev <id> -ncand CI) manually reconstructsinputOptionsfromgetConfig(), picking onlyexternal/input/plugins/context/cache— silently dropping theonwarnhandler that suppressesEVALandMODULE_LEVEL_DIRECTIVEwarnings. Watch mode spreads the entire config object, so it was never affected. This is what caused the "Use of eval... strongly discouraged" warning to keep appearing during-ncbuilds even thoughonwarnwas written specifically to suppress it back in Dec 2024. Added the missingonwarn: options.onwarnto the build-modeinputOptionsso both paths behave consistently.Verification
clickHandlers.jsunchanged (16, confirmed viagit stashcomparison — the guard doesn't introduce new type errors)jgclark.Dashboardbuilds clean vianpc plugin:dev jgclark.Dashboard -ncwith no eval warning (previously present every build)eslinton both changed files: clean🤖 Generated with Claude Code